Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extending and Modifying sorting feature of JUnit4. #882

Closed
wants to merge 1 commit into from

Conversation

fshepherd
Copy link

Adding SortWith Annotation to JUnit4 such that test methods defined in a
test class or test classes defined in a test suite can be executed in a
predefined order.

There are four sorting methods available currently for choosing. They are
default, jvm, name ascending and random. First three methods works exactly
as same as their ancestors defined in MethodSorter class. Random sorting
method is added due to the request of adding randomness to tests
execution. A test class annotated with random sorting method executes its
test methods in a random order. A test suites annotated with random
sorting method executes its test classes in a random order.

User still could specify sorting method manually by designating a sorting
request. Also the manually specified sorting method will override all
declarative sorting method defined previously.

Currently I disabled MethodSorter class temporarily because I think if
this patch was accepted, it would no longer be useful.

BTW this is my first attempt to be part of an open source project hence I
might omit something important. Please spare me in advance if I did
something wrong.

Adding SortWith Annotation to JUnit4 such that test methods defined in a
test class or test classes defined in a test suite can be executed in a
predefined order.

There are four sorting methods available currently for choosing. They are
default, jvm, name ascending and random. First three methods works exactly
as same as their ancestors defined in MethodSorter class. Random sorting
method is added due to the request of adding randomness to tests
execution. A test class annotated with random sorting method executes its
test methods in a random order. A test suites annotated with random
sorting method executes its test classes in a random order.

User still could specify sorting method manually by designating a sorting
request. Also the manually specified sorting method will override all
declarative sorting method defined previously.

Currently I disabled MethodSorter class temporarily because I think if
this patch was accepted, it would no longer be useful.

BTW this is my first attempt to be part of an open source project hence I
might omit something important. Please spare me in advance if I did
something wrong.
@@ -81,6 +81,7 @@ public void filter(Filter filter) throws NoTestsRemainException {
}

public void sort(Sorter sorter) {
sorter.apply(fRunner);
if(sorter!=null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we put spaces around the "if" keyword, and in new code we try to always use braces for if statements.

This same issue is in a few places

Note that code under org.junit will be migrated to the Google Java Style Guide; see http://google-styleguide.googlecode.com/svn/trunk/javaguide.html

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not being aware of coding style. Will clean up my code.

@kcooney
Copy link
Member

kcooney commented Apr 24, 2014

Thanks for the changes

Before we go further, we should figure out how @SortWith interacts with existing JUnit features

  • if a class A is annotated with @RunWith(Suite.class) and @SortWith, are all of the classes and methods that are (directly or indirectly) in the suite sorted? Or are only the ordering of the classes in A affected?
  • if class A is annotated with @RunWith(Suite.class) and @SortWith, and one of the contained classes (B) is also annotated with @SortWith, which sorting do you get for B?
  • for each of the above cases, what happens if you take the request and tell JUnit to sort the request?

Whatever we decide, we should document the behavior, and be sure to have tests to ensure we implement that behavior.

In addition, I feel that if a class is sorted with @SortWith JUnit should first sort the classes with the default sorter before applying the sort (this can be done by combining the comparators or as separate step). If we don't, and if you specify a sorter that does a partial ordering, the result will be non-deterministic on JDK 7 and above. For example, I could decide to write a sorter that orders all tests annotated with @UnitTest before all tests annotated with @IntegrationTest but otherwise keeps the ordering consistent. Assuming we use a stable sort, we want the end result to be deterministic.

@fshepherd
Copy link
Author

kcooney thanks for your suggestions. really helps! I think I did not make things clear...my bad...sorry for the confusion I have caused. Let me try to answer your questions.

For point 1, the current implementation does not sort the methods of a test case class included in a test suite as long as there is no @SortWith specified for that test case class. A test suite class annotated with @SortWith is only responsible for sorting the execution order of all test case classes involved. The class-level sorting is done by comparing names of two classes using a sorting method like JVM.

For point 2, B gets sorting method defined in the @SortWith annotation for B. The @SortWith annotation for test suite class only cares the execution order of all contained classes. How the methods included in a contained test case class is sorted is determined by the @SortWith annotation for that class.

For point 3, if a sorting request is made by users, the sorting method specified by it will override all other sorting methods declared by @SortWith annotation, no matter for class-level or method-level. In other words, sorting method specified manually by users through sorting request has the highest priority and will override every single declarative sorting method (both class-level and method-level) defined by @SortWith annotation.

For the last point, I agree with you. Should implement a default sorting strategy first to prevent test execution from being not deterministic if a sorter is specified and it does partial ordering.

@kcooney
Copy link
Member

kcooney commented Apr 25, 2014

@fshepherd To respond to your responses

For point 1, that seems surprising. That's not how the categories runner works, for example. The categories runner affects all test classes and test methods that are part of the class that is annotated. That's also not how it works when you tell a request to sort.

For point 3, I would expect the sorting made via the request to happen after the sorting specified via @SortWith or @FixMethodOrder. The reasoning is that the comparator passed into the request can return zero for two descriptions where it doesn't care which one is first. I think this is what you meant in your response, but I wasn't sure.

@Tibor17
Copy link
Contributor

Tibor17 commented Sep 7, 2014

How is @SortWith different from @FixMethodOrder ?
I can see the same enum. Why you did not simply add RANDOM to enum MethodSorters ?
People would like to specify a fixed order by themself. So why you did not add ORDINAL in the existing MethodSorters and @Ordinal(integer value = 0) on test method?

You know what i mean, our traditional problem of API design.
Instead of making FixMethodOrder generic in 4.11 and useful in Suite like this feature we may now mark FixMethodOrder @deprecated which means that FMO survived one release.
Other strategy which simplifies the code, because touching only Suite class, would be introducing new classes Suite.SuiteOrdersand Suite.FixSuiteOrder because we already started in 4.11 with this naming convention.

So what is better, to remove a lot of code in 5.0 and making developer angry or finding the way with not-duplicated features.

@kcooney
Copy link
Member

kcooney commented Sep 10, 2014

@Tibor17 I would want @SortWith to allow users to specify their own sorting algorithms.

I also don't think JUnit should have a built-in random sorting option.

@kcooney
Copy link
Member

kcooney commented Apr 25, 2015

I sent out pull #1130 which adds @SortWith.

@kcooney kcooney closed this Apr 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants